feat: Make toolbox and flyout focusable #8920

BenHenning · 2025-04-22T00:56:16Z

The basics

I validated my changes

The details

Resolves

Fixes #8918
Fixes #8919
Fixes part of #8771

Proposed Changes

This updates several classes in order to make toolboxes and flyouts focusable:

IFlyout is now an IFocusableTree with corresponding implementations in FlyoutBase.
IToolbox is now an IFocusableTree with corresponding implementations in Toolbox.
IToolboxItem is now an IFocusableNode with corresponding implementations in ToolboxItem.
As the primary toolbox items, ToolboxCategory and ToolboxSeparator were updated to have -1 tab indexes and defined IDs to help ToolboxItem fulfill its contracted for IFocusableNode.getFocusableElement.

Each of these two new focusable trees have specific noteworthy behaviors behaviors:

Toolbox will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen).
Toolbox will automatically synchronize its selection state with its item nodes being focused.
FlyoutBase, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a new tab stop being introduced.
FlyoutBase holds a workspace (for rendering blocks) and, since WorkspaceSvg is already set up to be a focusable tree, it's represented as a subtree to FlyoutBase. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience.
Both FlyoutBase and WorkspaceSvg have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the T/Esc flows supported in the keyboard navigation plugin playground.

One other thing to note: Toolbox had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of FocusManager's state guardrails).

Reason for Changes

This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin).

Test Coverage

No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915.

Documentation

No documentation changes should be needed here.

Additional Information

This includes changes that have been pulled from #8875.

This adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported.

This introduces the fundamental support needed to ensure that both toolboxes and flyouts are focusable using FocusManager.

…-workspace-focusable

…focusable

BenHenning

Self-reviewed changes not part of #8916 (though note that this includes WorkspaceSvg changes on top of that PR's changes).

BenHenning · 2025-04-22T01:08:39Z

PTAL @maribethb. Note that files changed in #8916 and #8909 can be ignored except for WorkspaceSvg (since both this and #8916 make changes to that file).

rachel-fenichel · 2025-04-23T15:49:15Z

@BenHenning would you like me to take over reviewing this one since Maribeth is out of office?

…-workspace-focusable

BenHenning · 2025-04-23T21:36:30Z

Yes please @rachel-fenichel but I will bring this up-to-date momentarily after #8916 has had a couple of updates, and will assign it over to you.

Addresses review comment.

…focusable

BenHenning · 2025-04-23T22:28:31Z

PTAL @rachel-fenichel.

rachel-fenichel · 2025-04-24T17:12:20Z

core/toolbox/toolbox.ts

+    // root of the toolbox.
+    if (!previousNode || previousNode === this) {
+      return this.getToolboxItems().find((item) => item.isSelectable()) ?? null;
+    } else return null;


No need for the else, just do a newline and return null

rachel-fenichel · 2025-04-24T17:17:09Z

core/workspace_svg.ts

+      if (flyout && nextTree === flyout) return;
+      if (toolbox && nextTree === toolbox) return;
+      if (toolbox) toolbox.clearSelection();
+      if (flyout && flyout instanceof Flyout) flyout.autoHide(false);


What else would flyout be besides an instanceof Flyout?

It's IFlyout in this context per: https://github.com/google/blockly/blob/ac7fea11cffc4c445cedae77675290861ceec2f6/core/workspace_svg.ts#L1014

I think logically it can only ever be Flyout but TypeScript doesn't know that.

I see. Makes sense.

It's IFlyout in this context per:

https://github.com/google/blockly/blob/ac7fea11cffc4c445cedae77675290861ceec2f6/core/workspace_svg.ts#L1014

I think logically it can only ever be Flyout but TypeScript doesn't know that.

Using our base Flyout class is technically not required, only using the IFlyout interface, and I’m pretty sure one of the partners doesn’t use our base class. Instead you should use the IAutoHideable interface we have. Which our base class implements but isn’t required

Addresses reviewer comment.

rachel-fenichel

One nit then LGTM!

rachel-fenichel · 2025-04-24T19:39:59Z

core/toolbox/toolbox.ts

+      if (this.getSelectedItem() !== node) {
+        this.setSelectedItem(node as IToolboxItem);
+      }
+    } else this.clearSelection();


Nit: use braces around the else

rachel-fenichel · 2025-04-24T19:40:48Z

core/workspace_svg.ts

+      if (flyout && nextTree === flyout) return;
+      if (toolbox && nextTree === toolbox) return;
+      if (toolbox) toolbox.clearSelection();
+      if (flyout && flyout instanceof Flyout) flyout.autoHide(false);


I see. Makes sense.

…focusable

Addresses a reviewer comment.

…focusable

BenHenning

Self-reviewed changes after merging #8916. I don't think another review round is necessary as the changes look clean.

Not really necessary, but I like seeing green CI before merging.

BenHenning

Empty commit review.

BenHenning · 2025-04-24T21:52:30Z

Thanks for the reviews! Going ahead and merging this to keep the downstream PRs simpler.

This reverts commit 5bc8380.

…n#8920)" This reverts commit 5bc8380.

* Revert "feat: Make toolbox and flyout focusable (#8920)" This reverts commit 5bc8380. * Revert "feat: Make WorkspaceSvg and BlockSvg focusable (#8916)" This reverts commit d7680cf.

## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes RaspberryPiFoundation#8918 Fixes RaspberryPiFoundation#8919 Fixes part of RaspberryPiFoundation#8771 ### Proposed Changes This updates several classes in order to make toolboxes and flyouts focusable: - `IFlyout` is now an `IFocusableTree` with corresponding implementations in `FlyoutBase`. - `IToolbox` is now an `IFocusableTree` with corresponding implementations in `Toolbox`. - `IToolboxItem` is now an `IFocusableNode` with corresponding implementations in `ToolboxItem`. - As the primary toolbox items, `ToolboxCategory` and `ToolboxSeparator` were updated to have -1 tab indexes and defined IDs to help `ToolboxItem` fulfill its contracted for `IFocusableNode.getFocusableElement`. Each of these two new focusable trees have specific noteworthy behaviors behaviors: - `Toolbox` will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen). - `Toolbox` will automatically synchronize its selection state with its item nodes being focused. - `FlyoutBase`, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a **new** tab stop being introduced. - `FlyoutBase` holds a workspace (for rendering blocks) and, since `WorkspaceSvg` is already set up to be a focusable tree, it's represented as a subtree to `FlyoutBase`. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience. - Both `FlyoutBase` and `WorkspaceSvg` have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the `T`/`Esc` flows supported in the keyboard navigation plugin playground. One other thing to note: `Toolbox` had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of `FocusManager`'s state guardrails). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of RaspberryPiFoundation#8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from RaspberryPiFoundation#8875.

_Note: This is a roll forward of #8920 that was reverted in #8933. See Additional Information below._ ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8918 Fixes #8919 Fixes part of #8943 Fixes part of #8771 ### Proposed Changes This updates several classes in order to make toolboxes and flyouts focusable: - `IFlyout` is now an `IFocusableTree` with corresponding implementations in `FlyoutBase`. - `IToolbox` is now an `IFocusableTree` with corresponding implementations in `Toolbox`. - `IToolboxItem` is now an `IFocusableNode` with corresponding implementations in `ToolboxItem`. - As the primary toolbox items, `ToolboxCategory` and `ToolboxSeparator` were updated to have -1 tab indexes and defined IDs to help `ToolboxItem` fulfill its contracted for `IFocusableNode.getFocusableElement`. - `FlyoutButton` is now an `IFocusableNode` (with corresponding ID generation, tab index setting, and ID matching for retrieval in `WorkspaceSvg`). Each of these two new focusable trees have specific noteworthy behaviors behaviors: - `Toolbox` will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen). - `Toolbox` will automatically synchronize its selection state with its item nodes being focused. - `FlyoutBase`, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a **new** tab stop being introduced. - `FlyoutBase` holds a workspace (for rendering blocks) and, since `WorkspaceSvg` is already set up to be a focusable tree, it's represented as a subtree to `FlyoutBase`. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience. - Both `FlyoutBase` and `WorkspaceSvg` have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the `T`/`Esc` flows supported in the keyboard navigation plugin playground. One other thing to note: `Toolbox` had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of `FocusManager`'s state guardrails). This only addresses part of #8943: it adds support for `FlyoutButton` which covers both buttons and labels. However, a longer-term solution may be to change `FlyoutItem` itself to force using an `IFocusableNode` as its element. ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. This was originally merged in #8916 but was reverted in #8933 due to RaspberryPiFoundation/blockly-keyboard-experimentation#481. Note that this does contain a number of differences from the original PR (namely, changes in `WorkspaceSvg` and `FlyoutButton` in order to make `FlyoutButton`s focusable). Otherwise, this has the same caveats as those noted in #8938 with regards to the experimental keyboard navigation plugin.

BenHenning added 5 commits April 21, 2025 23:36

feat: Make WorkspaceSvg focusable.

41c8215

This adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported.

chore: remove accidental 'test.only'.

14b486e

feat: Make Toolbox & Flyout focusable.

26cf8db

This introduces the fundamental support needed to ensure that both toolboxes and flyouts are focusable using FocusManager.

Merge branch 'add-focus-manager-callbacks-and-improvements' into make…

5ef2d7e

…-workspace-focusable

Merge branch 'make-workspace-focusable' into make-toolbox-and-flyout-…

996208d

…focusable

BenHenning commented Apr 22, 2025

View reviewed changes

BenHenning marked this pull request as ready for review April 22, 2025 01:07

BenHenning requested a review from a team as a code owner April 22, 2025 01:07

BenHenning requested a review from maribethb April 22, 2025 01:07

BenHenning assigned maribethb Apr 22, 2025

BenHenning mentioned this pull request Apr 22, 2025

[Do not merge] Test prototype focus manager incorporation #8875

Closed

1 task

sappm01 added the PR: feature Adds a feature label Apr 22, 2025

Merge branch 'add-focus-manager-callbacks-and-improvements' into make…

4479b82

…-workspace-focusable

BenHenning added 3 commits April 23, 2025 21:43

fix: Ensure Block paths are focusable.

2637736

chore: Fix line comment.

49192ba

Addresses review comment.

Merge branch 'make-workspace-focusable' into make-toolbox-and-flyout-…

917c4b6

…focusable

BenHenning assigned rachel-fenichel and unassigned maribethb Apr 23, 2025

BenHenning requested review from rachel-fenichel and removed request for maribethb April 23, 2025 22:28

rachel-fenichel reviewed Apr 24, 2025

View reviewed changes

chore: reduce branching.

d276dbc

Addresses reviewer comment.

rachel-fenichel approved these changes Apr 24, 2025

View reviewed changes

BenHenning added 3 commits April 24, 2025 21:25

fix: remove unnecessary shadow check.

a346a92

Merge branch 'make-workspace-focusable' into make-toolbox-and-flyout-…

4ed61bf

…focusable

chore: add braces.

80c8859

Addresses a reviewer comment.

BenHenning added 2 commits April 24, 2025 21:48

Merge branch 'rc/v12.0.0' into make-workspace-focusable

b3bd5e7

Merge branch 'make-workspace-focusable' into make-toolbox-and-flyout-…

1f0cefc

…focusable

BenHenning commented Apr 24, 2025

View reviewed changes

BenHenning changed the title ~~feat: Make toolbox and flyout focusable [Blocked on: #8916]~~ feat: Make toolbox and flyout focusable Apr 24, 2025

chore: empty commit to make CI pass.

c2384c6

Not really necessary, but I like seeing green CI before merging.

BenHenning commented Apr 24, 2025

View reviewed changes

BenHenning merged commit 5bc8380 into RaspberryPiFoundation:rc/v12.0.0 Apr 24, 2025
7 checks passed

BenHenning deleted the make-toolbox-and-flyout-focusable branch April 24, 2025 22:09

This was referenced Apr 24, 2025

Toolbox categories and subcategories. #8918

Closed

The flyout. #8919

Closed

RoboErikG added a commit that referenced this pull request Apr 25, 2025

Revert "feat: Make toolbox and flyout focusable (#8920)"

7fcad2a

This reverts commit 5bc8380.

RoboErikG mentioned this pull request Apr 25, 2025

Revert "feat: Make toolbox and flyout focusable" #8931

Closed

RoboErikG added a commit to RoboErikG/blockly that referenced this pull request Apr 25, 2025

Revert "feat: Make toolbox and flyout focusable (RaspberryPiFoundatio…

023358e

…n#8920)" This reverts commit 5bc8380.

BenHenning mentioned this pull request Apr 28, 2025

Fix: Revert focus prs #8933

Merged

1 task

BenHenning mentioned this pull request Apr 29, 2025

feat: Make toolbox and flyout focusable (roll forward) #8939

Merged

1 task

BenHenning restored the make-toolbox-and-flyout-focusable branch April 30, 2025 01:28

BenHenning mentioned this pull request Apr 30, 2025

feat: Make connections focusable #8928

Merged

1 task

maribethb mentioned this pull request May 5, 2025

fix: focus for autohideable flyouts #8990

Merged

1 task

feat: Make toolbox and flyout focusable #8920

feat: Make toolbox and flyout focusable #8920

Uh oh!

Conversation

BenHenning commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

BenHenning commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rachel-fenichel commented Apr 23, 2025

Uh oh!

BenHenning commented Apr 23, 2025

Uh oh!

BenHenning commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

BenHenning left a comment

Choose a reason for hiding this comment

Uh oh!

BenHenning commented Apr 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BenHenning commented Apr 22, 2025 •

edited

Loading

BenHenning commented Apr 22, 2025 •

edited

Loading